-
-
Notifications
You must be signed in to change notification settings - Fork 560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes delegates created from expressions. #585
Conversation
…e as their first argument, causing mapping of parameters to fail.
@@ -37,6 +38,12 @@ public DelegateWrapper(Engine engine, Delegate d) | |||
public override JsValue Call(JsValue thisObject, JsValue[] jsArguments) | |||
{ | |||
var parameterInfos = _d.Method.GetParameters(); | |||
|
|||
if (parameterInfos.Length > 0 && parameterInfos[0].ParameterType.FullName == "System.Runtime.CompilerServices.Closure") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is typeof possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first inclination, but System.Runtime.CompilerServices.Closure doesn't exist in dotnet framework - I tried adding and using pre-processor directives to only check if dotnet standard, but it didn't work with testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have your tried wrapping the code with
#if NETFRAMEWORK
... the code ...
#endif
Bot test case and this addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had only done it on the addition before - I just applied it to both and now it works, but it is basically ignoring the test case, which is probably not desirable. Does it get tested with both flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add this line to appveyor.yml which is in the root folder:
- dotnet test .\Jint.Tests\Jint.Tests.csproj -c Release -f net452
That should include the test in CI builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
|
||
if (parameterInfos.Length > 0 && parameterInfos[0].ParameterType.FullName == "System.Runtime.CompilerServices.Closure") | ||
{ | ||
parameterInfos = parameterInfos.Skip(1).ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skip is a bit nasty as it uses LINQ and doesn't perform that well, especially on full framework, but this is fortunately quite isolated case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to more performantly use Array.Copy, it looks like it is about 0.1 ticks faster on average, but is a fair amount more complicated. Alternatively I could use ArraySegment<> (or Span<> in C# 7), but that would change the signature downstream and make the non-NETFRAMEWORK case less performant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was bit of nitpicking, in Jint codebase there should be as little using System.Linq;
as possible, which I see you kept 😉 LINQ is much wiser on .NET Core and can be used more safely, but it still adds some penalty - especially on allocation side. So ideally following the Roslyn ideology of not using it at all if possible.
Thanks for considering this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed lol.
I'm used to code bases where an extra object allocation is the least of it's sins 😄
…rage and only runs the test and check in NETFRAMEWORK
Fixes bug where delegates generated from expressions contain a closure as their first argument, causing mapping of parameters to fail.